Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: trip vs shape distance validation #1553

Merged
merged 5 commits into from
Sep 25, 2023
Merged

feat: trip vs shape distance validation #1553

merged 5 commits into from
Sep 25, 2023

Conversation

cka-y
Copy link
Contributor

@cka-y cka-y commented Aug 7, 2023

Summary:

Created trip_distance_exceeds_shape_distancenotice when the max value shapeDistTraveled of a trip is greater than the max value of shapeDistTraveledfor its associated shape.

Closes #163

Expected behavior:

Screenshot 2023-08-07 at 1 10 57 AM

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@cka-y
Copy link
Contributor Author

cka-y commented Aug 7, 2023

@emmambd could you confirm what fields should be present in the notice's details?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

❌ Invalid acceptance test.
New Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 107 out of 1439 datasets (~7%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1439 sources (~0 %) are corrupted.
Commit: 712af2a
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

@emmambd
Copy link
Contributor

emmambd commented Aug 8, 2023

@cka-y List of fields is correct! This is enough to cross reference the error and find what the problem is.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

❌ Invalid acceptance test.
New Errors: 107 out of 1439 datasets (~7%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 1 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1439 sources (~0 %) are corrupted.
Commit: 014790d
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

Copy link
Contributor

@qcdyx qcdyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@emmambd emmambd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing some spot checks and looking at the report, this looks good to me! Noting that the acceptance tests generate errors for 7% of feeds in the Mobility Database which is above the recommended threshold, but this rule does align with the community consensus from https://github.com/google/transit/pull/380/files. @cka-y One question, not a blocker: any obvious reason why the list of feeds in the acceptance report is slightly different from the one in the original Python analysis report?

Copy link
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nicely done!

import org.mobilitydata.gtfsvalidator.table.*;

@RunWith(JUnit4.class)
public class TripAndShapeDistanceValidatorTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition!

@cka-y cka-y merged commit 176e654 into master Sep 25, 2023
333 of 334 checks passed
@cka-y cka-y deleted the feat/163 branch September 25, 2023 11:36
@cka-y
Copy link
Contributor Author

cka-y commented Sep 25, 2023

@emmambd seems like the script was using the producer URL i.e. current version of the dataset as opposed to the action that uses a persisted version of the dataset. I'm going to keep an eye on this but I think it's safe to merge atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement trip distance vs. shape verification (GTFS rule)
4 participants